Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(log): refactor some logs #1899

Closed
wants to merge 1 commit into from
Closed

fix(log): refactor some logs #1899

wants to merge 1 commit into from

Conversation

rchincha
Copy link
Contributor

@rchincha rchincha commented Oct 6, 2023

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rchincha
Copy link
Contributor Author

rchincha commented Oct 6, 2023

ac3801e (Ramkumar Chinchani 2021-12-13 19:23:31 +0000)│ 1224 rh.c.Log.Info().Int64("r.ContentLength", request.ContentLength).Msg("DEBUG")

:(

@rchincha rchincha force-pushed the log branch 4 times, most recently from 7ea00c5 to 9debeca Compare October 7, 2023 05:27
andaaron
andaaron reviewed Oct 7, 2023
View reviewed changes
Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests need updates, there are many tests for background tasks waiting on specific log messages.

pkg/cli/server/root.go Outdated
@@ -321,7 +321,7 @@ func validateExtensionsConfig(cfg *config.Config, log zlog.Logger) error {
//nolint:lll
if subPath.StorageDriver != nil && cfg.Extensions != nil && cfg.Extensions.Search != nil &&
cfg.Extensions.Search.Enable != nil && *cfg.Extensions.Search.Enable && cfg.Extensions.Search.CVE != nil {
log.Error().Err(zerr.ErrBadConfig).Msg("CVE functionality can't be used with remote storage. Please disable CVE")
log.Error().Err(zerr.ErrBadConfig).Msg("cveCVE functionality can't be used with remote storage. Please disable cve")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error().Err(zerr.ErrBadConfig).Msg("cveCVE functionality can't be used with remote storage. Please disable cve")
log.Error().Err(zerr.ErrBadConfig).Msg("cve functionality can't be used with remote storage. Please disable cve")

pkg/extensions/extension_search.go Outdated
@@ -59,7 +59,7 @@ func EnableSearchExtension(conf *config.Config, storeController storage.StoreCon
func downloadTrivyDB(interval time.Duration, sch *scheduler.Scheduler, cveScanner CveScanner, log log.Logger) {
generator := cveinfo.NewDBUpdateTaskGenerator(interval, cveScanner, log)

log.Info().Msg("Submitting CVE DB update scheduler")
log.Info().Msg("submitting CVE DB update scheduler")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Info().Msg("submitting CVE DB update scheduler")
log.Info().Msg("submitting CVE DB update generator to scheduler")

Was my mistake

pkg/extensions/extension_search.go Outdated
@@ -68,7 +68,7 @@ func startScanner(interval time.Duration, metaDB mTypes.MetaDB, sch *scheduler.S
) {
generator := cveinfo.NewScanTaskGenerator(metaDB, cveScanner, log)

log.Info().Msg("Submitting CVE scan scheduler")
log.Info().Msg("submitting CVE scan scheduler")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Info().Msg("submitting CVE scan scheduler")
log.Info().Msg("submitting CVE scan generator to scheduler")

pkg/extensions/search/cve/scan.go Outdated
) scheduler.TaskGenerator {
sublogger := logC.With().Str("component", "search").Logger()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use component cve instead? These are background tasks after all.

Suggested change
sublogger := logC.With().Str("component", "search").Logger()
sublogger := logC.With().Str("component", "cve").Logger()

pkg/meta/parse.go
@@ -56,7 +56,7 @@ func ParseRepo(repo string, metaDB mTypes.MetaDB, storeController storage.StoreC

indexBlob, err := imageStore.GetIndexContent(repo)
if err != nil {
log.Error().Err(err).Str("repository", repo).Msg("load-repo: failed to read index.json for repo")
log.Error().Err(err).Str("repository", repo).Msg("failed to read index.json for repo")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error().Err(err).Str("repository", repo).Msg("failed to read index.json for repo")
log.Error().Err(err).Str("component", "metadb").Str("repository", repo).Msg("failed to read index.json for repo")

Copy link
Contributor Author

@rchincha rchincha Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to automate this better by creating a per-component sublogger. Let's do this in a separate PR

Copy link
Contributor

@andaaron andaaron Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just keep the file name and line number in the log msg? Instead of using a component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"component" would be a nice column to index
Many files map to a single component and they may get refactored around.

pkg/meta/parse.go
@@ -294,14 +294,14 @@ func getNotationSignatureLayersInfo(
var manifestContent ispec.Manifest
if err := json.Unmarshal(manifestBlob, &manifestContent); err != nil {
log.Error().Err(err).Str("repository", repo).Str("reference", manifestDigest).Msg(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error().Err(err).Str("repository", repo).Str("reference", manifestDigest).Msg(
log.Error().Err(err).Str("component", "metadb").Str("repository", repo).Str("reference", manifestDigest).Msg(

pkg/meta/parse.go
@@ -294,14 +294,14 @@ func getNotationSignatureLayersInfo(
var manifestContent ispec.Manifest
if err := json.Unmarshal(manifestBlob, &manifestContent); err != nil {
log.Error().Err(err).Str("repository", repo).Str("reference", manifestDigest).Msg(
"load-repo: unable to marshal blob index")
"failed to marshal blob index")

return layers, err
}

if len(manifestContent.Layers) != 1 {
log.Error().Err(zerr.ErrBadManifest).Str("repository", repo).Str("reference", manifestDigest).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error().Err(zerr.ErrBadManifest).Str("repository", repo).Str("reference", manifestDigest).
log.Error().Err(zerr.ErrBadManifest).Str("component", "metadb").Str("repository", repo).Str("reference", manifestDigest).

pkg/meta/parse.go
@@ -316,7 +316,7 @@ func getNotationSignatureLayersInfo(
layerContent, err := imageStore.GetBlobContent(repo, layer)
if err != nil {
log.Error().Err(err).Str("repository", repo).Str("reference", manifestDigest).Str("layerDigest", layer.String()).Msg(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error().Err(err).Str("repository", repo).Str("reference", manifestDigest).Str("layerDigest", layer.String()).Msg(
log.Error().Err(err).Str("component", "metadb").Str("repository", repo).Str("reference", manifestDigest).Str("layerDigest", layer.String()).Msg(

pkg/storage/imagestore/imagestore.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peusebiu please double check if the errors in this file are errors or warnings.

pkg/scheduler/scheduler.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these error messages I would add the component, as they may be confusing for someone not familiar with the code when looking at the log messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to refactoring this better. How to pass a sublogger into a "component"

@rchincha rchincha force-pushed the log branch 4 times, most recently from 896820a to 6feaf51 Compare October 10, 2023 03:21
@rchincha rchincha force-pushed the log branch 3 times, most recently from cb7e0c7 to c46aa1e Compare October 10, 2023 19:51
@rchincha
fix(log): refactor some logs
df51b43 
Signed-off-by: Ramkumar Chinchani <[email protected]>
@rchincha rchincha added this to the v2.0.0 milestone Nov 1, 2023
@rchincha
Copy link
Contributor Author

Closing this in favor on #2045

@rchincha rchincha closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andaaron andaaron andaaron left review comments

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk Awaiting requested review from eusebiu-constantin-petu-dbk

@shimish2 shimish2 Awaiting requested review from shimish2 shimish2 is a code owner

@adodon2go adodon2go Awaiting requested review from adodon2go adodon2go is a code owner

@Andreea-Lupu Andreea-Lupu Awaiting requested review from Andreea-Lupu Andreea-Lupu is a code owner

Assignees

@laurentiuNiculae laurentiuNiculae

@rchincha rchincha

Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rchincha @andaaron @laurentiuNiculae